Codecov Report

❌ Patch coverage is 98.91304% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.88%. Comparing base (4218a6d) to head (86e2a58).

@@               Coverage Diff               @@
##           ros2-master     #436      +/-   ##
===============================================
+ Coverage        77.72%   78.88%   +1.15%     
===============================================
  Files               29       29              
  Lines             2007     1894     -113     
  Branches           128      122       -6     
===============================================
- Hits              1560     1494      -66     
+ Misses             379      333      -46     
+ Partials            68       67       -1     

I had a couple of comments:

1. This is a result of the previous PR but the update of the i-term is delayed by one sample iteration since it is updated after the command is calculated. It may not be a big deal in the big picture, but it seems like if the error changes the changes should be applied in the same sample iteration not the next sample. It seems like the code could be modified to apply that change immediately although it might make the code slightly more complex. It just leaves a slightly bad taste when the changes are not applied in the current iteration.

2. I am probably in the minority but I think there are use cases where you want to limit the contribution of the i-term to the control command i.e. there are cases where you might still want a i_limit (min/max) outside of the anti-windup strategy.

I had a couple of comments:

1. This is a result of the previous PR but the update of the i-term is delayed by one sample iteration since it is updated after the command is calculated. It may not be a big deal in the big picture, but it seems like if the error changes the changes should be applied in the same sample iteration not the next sample. It seems like the code could be modified to apply that change immediately although it might make the code slightly more complex. It just leaves a slightly bad taste when the changes are not applied in the current iteration.

See the discussion above about forward and backward integration of the integral action.

2. I am probably in the minority but I think there are use cases where you want to limit the contribution of the i-term to the control command i.e. there are cases where you might still want a i_limit (min/max) outside of the anti-windup strategy.

I've already cited the Matlab/Simulink implementation above, and it also has an I_max/I_min limit. Maybe we should add this again, @ViktorCVS what do you think?

I had a couple of comments:

1. This is a result of the previous PR but the update of the i-term is delayed by one sample iteration since it is updated after the command is calculated. It may not be a big deal in the big picture, but it seems like if the error changes the changes should be applied in the same sample iteration not the next sample. It seems like the code could be modified to apply that change immediately although it might make the code slightly more complex. It just leaves a slightly bad taste when the changes are not applied in the current iteration.

See the discussion above about forward and backward integration of the integral action.

2. I am probably in the minority but I think there are use cases where you want to limit the contribution of the i-term to the control command i.e. there are cases where you might still want a i_limit (min/max) outside of the anti-windup strategy.

I've already cited the Matlab/Simulink implementation above, and it also has an I_max/I_min limit. Maybe we should add this again, @ViktorCVS what do you think?

It’s not a problem to add this feature again, but I really can’t see any utility in it since the controller is digital and, as I mentioned, it’s a form of conditional integration that is less effective and harder to tune (according to various sources). Since MATLAB implemented it and we would still have it without this PR, it could be an option to remove it as a strategy and treat it the same way as the output clamp: on by default with default limits at infinity.
If you agree, I can change the entire PR to accomplish that.

yes please, no additional parameter but setting i_limits to infinity.

I had a couple of comments:

1. This is a result of the previous PR but the update of the i-term is delayed by one sample iteration since it is updated after the command is calculated. It may not be a big deal in the big picture, but it seems like if the error changes the changes should be applied in the same sample iteration not the next sample. It seems like the code could be modified to apply that change immediately although it might make the code slightly more complex. It just leaves a slightly bad taste when the changes are not applied in the current iteration.

See the discussion above about forward and backward integration of the integral action.

2. I am probably in the minority but I think there are use cases where you want to limit the contribution of the i-term to the control command i.e. there are cases where you might still want a i_limit (min/max) outside of the anti-windup strategy.

I've already cited the Matlab/Simulink implementation above, and it also has an I_max/I_min limit. Maybe we should add this again, @ViktorCVS what do you think?

Oh sorry. I didn't see the discussion on forward and backward integration. So the plan is to change it to forward integration in another PR - is that correct?

And I agree, adding a simple clamp for imax/imin is fine with default set to infinity. I assume you will then maintain the i_clamp_min/max ros parameters in PidROS.

@ViktorCVS Please also see related issue I just opened: #442

@ViktorCVS did you close this on purpose? I think it was almost ready for merge?

@ViktorCVS did you close this on purpose? I think it was almost ready for merge?

I didn't close it, but I started from scratch. I've already done the main code, only the tests are left. Maybe when I commit it opens again

It is compiling, I'll fix the tests and change it from draft to ready for review.

This pull request is in conflict. Could you fix it @ViktorCVS?